fix: Prevent EXISTS from incrementing keyspace hit/miss stats#3986
fix: Prevent EXISTS from incrementing keyspace hit/miss stats#3986yulazariy wants to merge 1 commit into
Conversation
Add LOOKUP_NOSTATS flag to lookupKeyReadWithFlags in existsCommand so that EXISTS does not affect keyspace_hits or keyspace_misses counters. These stats are meant for cache hit/miss tracking and EXISTS is a key existence check, not a cache access. Signed-off-by: Yehonatan Yulazari <yulazari@amazon.com>
📝 WalkthroughWalkthrough
EXISTS keyspace stats suppression
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/keyspace.tcl`:
- Line 76: The test at line 76 assumes that nonexistingkey is absent when
calling r exists nonexistingkey, but does not explicitly ensure this beforehand.
This can cause test failures if prior test state leaks and the key actually
exists, preventing the miss path from being exercised. Before the EXISTS call on
nonexistingkey, explicitly delete the key using r del nonexistingkey or assert
its absence to guarantee deterministic behavior and ensure the miss path is
always tested regardless of prior test state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8212c92c-acf2-40a1-9bae-102e6dddc662
📒 Files selected for processing (2)
src/db.ctests/unit/keyspace.tcl
| set hits_before [getInfoProperty $info keyspace_hits] | ||
| set misses_before [getInfoProperty $info keyspace_misses] | ||
| r exists existskey | ||
| r exists nonexistingkey |
There was a problem hiding this comment.
Make the miss-path assertion deterministic.
At Line 76, nonexistingkey is assumed absent; if prior test state leaks, this can become a hit and stop exercising the miss path. Explicitly delete/assert absence before the EXISTS call.
Suggested patch
test {EXISTS does not update keyspace hits or misses} {
r set existskey value
+ r del nonexistingkey
set info [r info stats]
set hits_before [getInfoProperty $info keyspace_hits]
set misses_before [getInfoProperty $info keyspace_misses]
r exists existskey
r exists nonexistingkey📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r exists nonexistingkey | |
| test {EXISTS does not update keyspace hits or misses} { | |
| r set existskey value | |
| r del nonexistingkey | |
| set info [r info stats] | |
| set hits_before [getInfoProperty $info keyspace_hits] | |
| set misses_before [getInfoProperty $info keyspace_misses] | |
| r exists existskey | |
| r exists nonexistingkey |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/keyspace.tcl` at line 76, The test at line 76 assumes that
nonexistingkey is absent when calling r exists nonexistingkey, but does not
explicitly ensure this beforehand. This can cause test failures if prior test
state leaks and the key actually exists, preventing the miss path from being
exercised. Before the EXISTS call on nonexistingkey, explicitly delete the key
using r del nonexistingkey or assert its absence to guarantee deterministic
behavior and ensure the miss path is always tested regardless of prior test
state.
|
Thank you @yulazariy IMO this makes sense. I do think this is makes sense, but it is also a behavioral change (even if never documented) |
|
cool thanks @ranshid Also looks like there is a (not related) flakey test https://github.com/valkey-io/valkey/actions/runs/27496365556/job/81271055172?pr=3986 |
|
@valkey-review-bot Is this a backward compatible change according to semantic versioning? |
|
On the backward-compatibility question (@zuiderkwast): this is a behavioral change to a public, observable interface, not an API-breaking one.
So: API-compatible, but a metric-semantics change. Under semver this fits a minor release (or a patch with an explicit release note), not a silent patch — consistent with @ranshid flagging it as a major-decision. One thing worth settling before merge: the change leaves One inline note on a side effect of the chosen flag. |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
|
|
||
| for (j = 1; j < c->argc; j++) { | ||
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH)) count++; | ||
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS)) count++; |
There was a problem hiding this comment.
LOOKUP_NOSTATS suppresses the keyspace_misses counter, but the keymiss keyspace notification is gated separately on LOOKUP_NONOTIFY (src/db.c:120-121), which is not passed here. So after this change EXISTS on a missing key still fires a NOTIFY_KEY_MISS (keymiss) event while no longer incrementing keyspace_misses — the two were previously in lockstep for this command.
If the intent is "EXISTS is not a cache access," the notification arguably should be suppressed too, for consistency with the stats:
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS)) count++; | |
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS | LOOKUP_NONOTIFY)) count++; |
If firing keymiss for EXISTS is intentional (some clients rely on it for key-tracking/invalidation), then leaving it as-is is fine — but worth confirming, since the divergence is easy to miss.
There was a problem hiding this comment.
@valkey-review-bot good catch about the LOOKUP_NONOTIFY. what other implications will there be if we add that one as well?
|
@valkey-review-bot Whether the behavior change breaks the documented behavior depends on whether this behavior is publicly documented for this command and these stats, right? The public documentation is in the valkey-doc repo. |
|
On the documentation question (@zuiderkwast): I can't inspect the
So whether this "breaks documented behavior" reduces entirely to what valkey-doc says about these two INFO fields and whether per-command contribution is described there. If valkey-doc only describes the counters generically (cache hit/miss tracking) without enumerating which commands feed them, this is a semantics shift but not a contradiction of a written contract; if it explicitly lists One code-level note that bears on framing this purely as a "stats" change: with |
enjoy-binbin
added
the
release-notes
@valkey-review-bot I think it makes sense that |
|
@ranshid — answering your follow-up about the implications of also passing I agree with your reasoning that |
|
|
||
| for (j = 1; j < c->argc; j++) { | ||
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH)) count++; | ||
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS)) count++; |
There was a problem hiding this comment.
On what adding LOOKUP_NONOTIFY would change here — the keymiss event has exactly two consumers, and tracking is not one of them:
- Keyspace pub/sub: subscribers of
__keyevent@<db>__:keymiss(i.e. operators running withminnotify-keyspace-events) would stop seeing an event whenEXISTSprobes a missing key. This branch is config-gated atsrc/notify.c:131-157. - Modules:
moduleNotifyKeyspaceEvent(src/notify.c:124) fires unconditionally, so any module subscribed viaVALKEYMODULE_NOTIFY_KEY_MISSwould stop getting the callback forEXISTSmisses. - Client-side caching / invalidation is unaffected. Key tracking is driven by the
CMD_READONLYflag insrc/server.c:4083-4091(trackingRememberKeys), independent of bothkeyspace_missesand thekeymissnotification — so neither this PR nor addingLOOKUP_NONOTIFYchanges what tracking-enabled clients get invalidated on.
So the only behavior at stake is the keymiss keyspace event / module notification, not invalidation. If the intent is "EXISTS is a pure existence probe that produces no observable side effects," the codebase already has a name for that exact combination:
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS)) count++; | |
| if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOEFFECTS)) count++; |
LOOKUP_NOEFFECTS is defined at src/server.h:3753 as LOOKUP_NONOTIFY | LOOKUP_NOSTATS | LOOKUP_NOTOUCH | LOOKUP_NOEXPIRE. Using it would keep stats and the notification in lockstep (both suppressed) and match the documented "avoid any effects from fetching the key" intent. If instead you want EXISTS to keep emitting keymiss (e.g. to preserve the module/pub-sub signal that a probe occurred) while only dropping the counter, the current LOOKUP_NOTOUCH | LOOKUP_NOSTATS is fine — but that intentional split is worth a one-line comment, since the next reader will see stats and notification diverge and assume it was an oversight.
|
I don't think this is a real bug and I don't think we should change it unless someone is actually complaining about it. If we want to go do the effort to more comprehensively define what a cache hit/cache miss is, then I might be OK. But right now it's really just "You accessed a key and it was there". Which feels like exists. |
|
In AWS we got a feedback from several customers that are look into the cache hit/cache miss metrics and find it miss-leading as they use exists before actually retrieving keys from the cache. |
5 participants
Add LOOKUP_NOSTATS flag to lookupKeyReadWithFlags in existsCommand so that EXISTS does not affect keyspace_hits or keyspace_misses counters. These stats are meant for cache hit/miss tracking and EXISTS is a key existence check, not a cache access.